Skip to content

Conversation

@timw
Copy link

@timw timw commented Jan 28, 2026

Provide a CorsSettings builder method to allow additional header ranges to be added, and the underlying ability to concatenate HttpHeaderRange instances to support it.

This supports scenarios where complex CorsSettings defaults are provided by downstream libraries (e.g. pekko-grpc) but still need to be extended/adapted by end user applications.

A concrete motivating example is the pekko-grpc defaultCorsSettings, which include:

  val defaultCorsSettings: CorsSettings = CorsSettings(ConfigFactory.load())
    .withAllowCredentials(true)
    .withAllowedMethods(immutable.Seq(HttpMethods.POST, HttpMethods.OPTIONS))
    .withExposedHeaders(immutable.Seq(headers.`Status`.name, headers.`Status-Message`.name, `Content-Encoding`.name))
    .withAllowedHeaders(
      HttpHeaderRange(
        "x-user-agent",
        "x-grpc-web",
        `Content-Type`.name,
        Accept.name,
        "grpc-timeout",
        `Accept-Encoding`.name))

In one of our (development) environments, we need the Authorization header to be added to the allowed headers range, but the withAllowedHeaders method only works by override, requiring us to either maintain our own distinct set of headers (which could diverge from the upstream) or do hacky matches on the unsealed HttpHeaderRange type.

This is a proposal for discussion on a more ergonomic/robust way to extend default settings like this.
The same pattern could equally be applied to the HTTP origin matchers and possibly the exposed headers.

I've tried to make the implementation in-line with conventions (e.g. I would prefer to seal scaladsl.model.HttpHeaderRange and have all the matches over it be required exhaustive, but the rest of pekko-http isn't really that way inclined), and the concat implementation is also awkward as exposing it to the Java DSL means you have to deal with those type params in the Scala methods and discourage extension.

Happy to discuss and rework if maintainers have opinions/better ideas about approach here.

Copy link
Member

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should target main branch, not 1.4.

When it gets merged, we can consider backporting to 1.4.

The code doesn't compile in Scala 2.12 and causes binary compatibility issues for other Scala versions. This will be less of an issue in main branch (no Scala 2.12 support, for example).

@timw
Copy link
Author

timw commented Jan 28, 2026

This should target main branch, not 1.4.

When it gets merged, we can consider backporting to 1.4.

Will rebase and re-push once you've had a chance for an initial comment.

The code doesn't compile in Scala 2.12 and causes binary compatibility issues for other Scala versions. This will be less of an issue in main branch (no Scala 2.12 support, for example).

Looks like that was due to the import changes (I haven't got much 2.12 experience TBH, so that's a bit mysterious) - will tidy all that up and check.

Will also have a think about binary compatibility - that might be harder to avoid if the concat API is to be available to the Java DSL, but it could be shunted to a utility method. The javadsl/scaladsl split does make things a bit suboptimal WRT sealing the types, so happy to take suggestions on the typical idioms there.

@pjfanning
Copy link
Member

You can add an excludes file for the mima failures in main branch.

@timw timw force-pushed the 1.4-header-range-concat branch from ef5f9f4 to 2f72cf9 Compare January 28, 2026 09:20
@timw timw changed the base branch from 1.4.x to main January 28, 2026 09:20
@timw
Copy link
Author

timw commented Jan 28, 2026

Rebased this on main and changed the target branch for this PR - also fixed the compilation and import issues (have run validatePullRequest and full +compile locally).

@timw timw marked this pull request as ready for review January 28, 2026 11:00
@timw
Copy link
Author

timw commented Jan 29, 2026

I've removed new method on CorsSettings as it's not strictly needed.
Clients of the API can add extension methods if they prefer to smooth out that syntax.

range match {
case `*` => `*`
case Default(headers) => Default(this.headers ++ headers)
case _ => throw new IllegalArgumentException(s"Unexpected header range implementation type ${range.getClass}")
Copy link
Member

@pjfanning pjfanning Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the error message be more like s"Concat is not supported for unexpected header range implementation type ${range.getClass}"?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've actually switched this over to use JavaMapping.toScala, which is consistent with other param casting in the Scala DSLs.
Let me know what you think.

Provide a CorsSettings builder method to allow additional header ranges to be added, and the underlying ability to concatenate HttpHeaderRange instances to support it.
This supports scenarios where complex CorsSettings defaults are provided by downstream libraries (e.g. pekko-grpc) but still need to be extended/adapted by end user applications.
@timw timw force-pushed the 1.4-header-range-concat branch from 2f72cf9 to 0de6d0b Compare January 29, 2026 09:44
Copy link
Member

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I work prefer to see other contributors' opinion before merging this.

Ideally, the public methods should have javadoc with @since 2.0.0.

@timw
Copy link
Author

timw commented Jan 29, 2026

Ideally, the public methods should have javadoc with @SInCE 2.0.0.

Pushed update with this.

@timw timw force-pushed the 1.4-header-range-concat branch from e78b1b2 to 401aa84 Compare January 29, 2026 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants